Feat/discord 에러로그 알림 도입#12
Hidden character warning
Conversation
There was a problem hiding this comment.
Pull request overview
이 PR은 디스코드 웹훅을 통한 에러 로그 알림 기능을 도입합니다. Logback 커스텀 appender를 구현하여 ERROR 레벨 이상의 로그를 디스코드 채널로 전송합니다.
- 커스텀 DiscordWebhookAppender 구현 (HTTP 클라이언트, 재시도 로직, 메시지 포맷팅)
- Logback-spring.xml 설정을 통한 비동기 로그 전송 구성
- Rate limiting(429) 응답 처리 및 자동 재시도 메커니즘 추가
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| src/main/resources/logback-spring.xml | 디스코드 웹훅 appender 설정 및 AsyncAppender를 통한 비동기 ERROR 로그 전송 구성 |
| src/main/java/me/gg/pinit/infrastructure/logging/DiscordWebhookAppender.java | 디스코드 웹훅 API 호출을 담당하는 커스텀 Logback appender 구현 (HTTP 요청, 재시도, 메시지 포맷팅, 예외 처리) |
| <root level="INFO"> | ||
| <appender-ref ref="ASYNC_DISCORD"/> | ||
| </root> |
There was a problem hiding this comment.
(1) 문제점: 환경변수 DISCORD_WEBHOOK_URL이 설정되지 않았을 때, 빈 문자열("")이 주입되어 start() 메서드가 경고만 남기고 appender가 비활성화됩니다. 그러나 AsyncAppender는 이미 root logger에 연결되어 있어 불필요한 오버헤드가 발생합니다.
(2) 영향: 디스코드 웹훅이 설정되지 않은 환경(로컬 개발 등)에서도 AsyncAppender가 동작하여 성능에 영향을 줄 수 있습니다.
(3) 수정 제안: 디스코드 알림이 필요한 환경(예: prod 프로파일)에서만 appender를 활성화하도록 springProfile 설정을 추가하세요.
| private int requestTimeoutMillis = 3000; | ||
|
|
||
| private HttpClient client; | ||
| private final ObjectMapper om = new ObjectMapper(); |
There was a problem hiding this comment.
(1) 문제점: ObjectMapper 인스턴스가 매 로그 이벤트마다 사용되지만, 스레드 안전성이 보장되지 않습니다. ObjectMapper는 기본적으로 스레드 안전하지만, 이 코드에서는 AsyncAppender를 사용하므로 여러 스레드에서 동시에 append() 메서드를 호출할 가능성이 있습니다.
(2) 영향: 실제로는 ObjectMapper가 스레드 안전하게 동작하지만, 코드 리뷰 관점에서 명시적으로 안전성을 보장하는 것이 좋습니다. 또한 향후 ObjectMapper 설정 변경 시 동시성 문제가 발생할 수 있습니다.
(3) 수정 제안: ObjectMapper를 final로 선언하고, 필요한 경우 주석으로 스레드 안전성을 명시하세요. 또는 매번 새로운 인스턴스를 생성하는 대신 공유 인스턴스 사용을 명확히 하세요.
| if (this.webhookUrl == null || this.webhookUrl.isEmpty()) { | ||
| addWarn("디스코드 웹훅 URL이 설정되지 않았습니다. DiscordWebhookAppender가 시작되지 않습니다."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
(1) 문제점: webhookUrl을 검증할 때 isEmpty()만 체크하고 있지만, 실제 URL 형식이 유효한지는 검증하지 않습니다. 잘못된 URL 형식이 설정되면 런타임에 URI.create()에서 예외가 발생할 수 있습니다.
(2) 영향: 잘못된 URL 설정 시 매 로그 이벤트마다 예외가 발생하여 성능 저하 및 불필요한 경고 로그가 반복적으로 생성됩니다.
(3) 수정 제안: start() 메서드에서 URI.create(webhookUrl)를 호출하여 URL 형식을 미리 검증하고, 유효하지 않으면 appender를 시작하지 않도록 수정하세요.
| private String format(ILoggingEvent event) { | ||
| String base = String.format( | ||
| "[%s] %-5s %s - %s", | ||
| java.time.Instant.ofEpochMilli(event.getTimeStamp()), | ||
| event.getLevel(), | ||
| event.getLoggerName(), | ||
| event.getFormattedMessage() | ||
| ); | ||
|
|
||
| if (event.getThrowableProxy() != null) { | ||
| String stack = ThrowableProxyUtil.asString(event.getThrowableProxy()); | ||
| return base + "\n```" + stack + "```"; | ||
| } | ||
| return base; | ||
| } |
There was a problem hiding this comment.
(1) 문제점: logback-spring.xml의 layout 설정과 DiscordWebhookAppender의 format() 메서드가 중복된 포맷팅 로직을 가지고 있습니다. layout 설정이 무시되고 format() 메서드의 포맷만 사용되고 있습니다.
(2) 영향: 설정 파일에서 포맷을 변경해도 실제로는 적용되지 않아 혼란을 초래할 수 있으며, 유지보수성이 떨어집니다.
(3) 수정 제안: layout 설정을 제거하거나, 또는 layout을 실제로 사용하도록 format() 메서드를 수정하여 getLayout().doLayout(event)를 호출하도록 변경하세요.
| public class DiscordWebhookAppender extends AppenderBase<ILoggingEvent> { | ||
|
|
||
| // logback-spring.xml 에서 주입 | ||
| @Setter | ||
| private String webhookUrl; | ||
| @Setter | ||
| private String username = "pinit-auth-log"; | ||
|
|
||
| // Discord content 제한 (2000자) | ||
| @Setter | ||
| private int maxContentLength = 1900; | ||
|
|
||
| @Setter | ||
| private int connectTimeoutMillis = 2000; | ||
| @Setter | ||
| private int requestTimeoutMillis = 3000; | ||
|
|
||
| private HttpClient client; | ||
| private final ObjectMapper om = new ObjectMapper(); | ||
|
|
||
| @Override | ||
| public void start() { | ||
| if (this.webhookUrl == null || this.webhookUrl.isEmpty()) { | ||
| addWarn("디스코드 웹훅 URL이 설정되지 않았습니다. DiscordWebhookAppender가 시작되지 않습니다."); | ||
| return; | ||
| } | ||
| client = HttpClient.newBuilder() | ||
| .connectTimeout(Duration.ofMillis(connectTimeoutMillis)) | ||
| .build(); | ||
|
|
||
| super.start(); | ||
| } | ||
|
|
||
| @Override | ||
| protected void append(ILoggingEvent event) { | ||
| if (!isStarted()) return; | ||
|
|
||
| try { | ||
| String content = format(event); | ||
| content = truncate(content, maxContentLength); | ||
|
|
||
| Map<String, Object> payload = Map.of( | ||
| "username", username, | ||
| "content", content, | ||
| "allowed_mentions", Map.of("parse", List.of()) | ||
| ); | ||
|
|
||
| String json = om.writeValueAsString(payload); | ||
|
|
||
| HttpRequest req = HttpRequest.newBuilder() | ||
| .uri(URI.create(webhookUrl)) | ||
| .timeout(Duration.ofMillis(requestTimeoutMillis)) | ||
| .header("Content-Type", "application/json") | ||
| .POST(HttpRequest.BodyPublishers.ofString(json)) | ||
| .build(); | ||
|
|
||
| HttpResponse<String> resp = client.send(req, HttpResponse.BodyHandlers.ofString()); | ||
|
|
||
| if (resp.statusCode() == 429) { | ||
| long waitMs = parseRetryAfterMillis(resp); | ||
| if (waitMs > 0) | ||
| Thread.sleep(waitMs); | ||
|
|
||
| HttpResponse<String> retry = client.send(req, HttpResponse.BodyHandlers.ofString()); | ||
| if (retry.statusCode() >= 200 && retry.statusCode() < 300) { | ||
| return; // 성공 | ||
| } | ||
| addWarn("DiscordWebhookAppender: 재시도 후에도 실패, 상태 코드: " + retry.statusCode()); | ||
| return; | ||
| } | ||
| if (resp.statusCode() < 200 || resp.statusCode() >= 300) { | ||
| addWarn("DiscordWebhookAppender: HTTP 요청 실패, 상태 코드: " + resp.statusCode()); | ||
| } | ||
| } catch (JsonProcessingException e) { | ||
| addWarn("DiscordWebhookAppender: JSON 직렬화 실패", e); | ||
| } catch (IOException e) { | ||
| addWarn("DiscordWebhookAppender: HTTP 요청 실패", e); | ||
| } catch (InterruptedException e) { | ||
| addWarn("DiscordWebhookAppender: HTTP 요청이 중단됨", e); | ||
| } | ||
| } | ||
|
|
||
| private String format(ILoggingEvent event) { | ||
| String base = String.format( | ||
| "[%s] %-5s %s - %s", | ||
| java.time.Instant.ofEpochMilli(event.getTimeStamp()), | ||
| event.getLevel(), | ||
| event.getLoggerName(), | ||
| event.getFormattedMessage() | ||
| ); | ||
|
|
||
| if (event.getThrowableProxy() != null) { | ||
| String stack = ThrowableProxyUtil.asString(event.getThrowableProxy()); | ||
| return base + "\n```" + stack + "```"; | ||
| } | ||
| return base; | ||
| } | ||
|
|
||
| private String truncate(String s, int max) { | ||
| if (s == null) return ""; | ||
| if (s.length() <= max) return s; | ||
| return s.substring(0, max) + "\n...(truncated)"; | ||
| } | ||
|
|
||
| private long parseRetryAfterMillis(HttpResponse<String> resp) { | ||
| String ra = resp.headers().firstValue("Retry-After").orElse(null); | ||
| if (ra == null || ra.isBlank()) return 0; | ||
|
|
||
| try { | ||
| double v = Double.parseDouble(ra.trim()); | ||
| return (long) (v * 1000); | ||
| } catch (NumberFormatException ignore) { | ||
| return 0; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
(1) 문제점: DiscordWebhookAppender 클래스에 대한 단위 테스트가 없습니다. HTTP 요청, 재시도 로직, 메시지 포맷팅 및 자르기, 예외 처리 등 여러 중요한 동작에 대한 테스트가 필요합니다.
(2) 영향: 향후 코드 수정 시 기존 기능이 깨질 수 있으며, 엣지 케이스(예: 긴 스택 트레이스, 특수 문자, 네트워크 타임아웃)가 제대로 처리되는지 확인할 수 없습니다.
(3) 수정 제안: MockWebServer 또는 WireMock을 사용하여 디스코드 웹훅 API를 모킹하고, 다음 시나리오에 대한 테스트를 추가하세요: 1) 정상 케이스, 2) 429 응답 및 재시도, 3) 네트워크 에러, 4) 메시지 길이 초과 시 자르기, 5) 잘못된 URL 처리.
| <appender name="ASYNC_DISCORD" class="ch.qos.logback.classic.AsyncAppender"> | ||
| <appender-ref ref="DISCORD"/> | ||
| <filter class="ch.qos.logback.classic.filter.ThresholdFilter"> | ||
| <level>ERROR</level> | ||
| </filter> | ||
| </appender> |
There was a problem hiding this comment.
(1) 문제점: AsyncAppender의 필터 설정이 잘못되었습니다. 현재 설정에서는 필터가 AsyncAppender 내부에 있어서 이벤트가 큐에 들어간 후에 필터링됩니다. 즉, ERROR 레벨이 아닌 로그도 큐에 쌓이게 됩니다.
(2) 영향: INFO, WARN 레벨의 모든 로그가 큐에 축적되어 메모리를 낭비하고, AsyncAppender의 큐가 불필요하게 채워질 수 있습니다.
(3) 수정 제안: ThresholdFilter를 DISCORD appender에 직접 적용하여 ERROR 레벨 미만의 이벤트가 큐에 들어가기 전에 필터링되도록 수정하세요.
| @Override | ||
| public void start() { | ||
| if (this.webhookUrl == null || this.webhookUrl.isEmpty()) { | ||
| addWarn("디스코드 웹훅 URL이 설정되지 않았습니다. DiscordWebhookAppender가 시작되지 않습니다."); | ||
| return; | ||
| } | ||
| client = HttpClient.newBuilder() | ||
| .connectTimeout(Duration.ofMillis(connectTimeoutMillis)) | ||
| .build(); | ||
|
|
||
| super.start(); | ||
| } |
There was a problem hiding this comment.
(1) 문제점: HttpClient 인스턴스가 start() 메서드에서 생성되지만, stop() 메서드에서 종료되지 않습니다. HttpClient는 내부적으로 ExecutorService와 ConnectionPool을 관리하므로 명시적으로 종료해야 합니다.
(2) 영향: 애플리케이션 종료 시 스레드 풀과 연결이 제대로 정리되지 않아 리소스 누수가 발생할 수 있습니다.
(3) 수정 제안: stop() 메서드를 오버라이드하여 client를 null로 설정하거나, 필요시 관련 리소스를 정리하도록 구현하세요.
| if (resp.statusCode() == 429) { | ||
| long waitMs = parseRetryAfterMillis(resp); | ||
| if (waitMs > 0) | ||
| Thread.sleep(waitMs); | ||
|
|
||
| HttpResponse<String> retry = client.send(req, HttpResponse.BodyHandlers.ofString()); | ||
| if (retry.statusCode() >= 200 && retry.statusCode() < 300) { | ||
| return; // 성공 | ||
| } | ||
| addWarn("DiscordWebhookAppender: 재시도 후에도 실패, 상태 코드: " + retry.statusCode()); | ||
| return; | ||
| } |
There was a problem hiding this comment.
(1) 문제점: append() 메서드가 동기적으로 HTTP 요청을 전송하고 있습니다. AsyncAppender를 사용하더라도, 디스코드 API가 느리거나 네트워크 문제가 발생하면 로깅 스레드가 블로킹되어 애플리케이션 성능에 영향을 줄 수 있습니다. 특히 429 응답 시 Thread.sleep()을 사용하여 동기적으로 대기합니다.
(2) 영향: 로깅 처리 지연으로 AsyncAppender의 큐가 가득 차면 로그가 유실될 수 있으며, 최악의 경우 애플리케이션 전체 성능에 영향을 줄 수 있습니다.
(3) 수정 제안: 429 응답을 받았을 때 재시도를 포기하고 경고만 남기거나, 또는 별도의 스레드 풀에서 비동기로 재시도를 처리하도록 개선하세요.
| } catch (InterruptedException e) { | ||
| addWarn("DiscordWebhookAppender: HTTP 요청이 중단됨", e); | ||
| } |
There was a problem hiding this comment.
(1) 문제점: InterruptedException을 catch한 후 스레드의 인터럽트 상태를 복원하지 않습니다.
(2) 영향: 스레드가 인터럽트되었다는 정보가 손실되어, 상위 레벨에서 스레드 종료를 적절히 처리하지 못할 수 있습니다. 특히 애플리케이션 종료 시 문제가 될 수 있습니다.
(3) 수정 제안: catch 블록에서 Thread.currentThread().interrupt()를 호출하여 인터럽트 상태를 복원하세요.
| if (s.length() <= max) return s; | ||
| return s.substring(0, max) + "\n...(truncated)"; |
There was a problem hiding this comment.
(1) 문제점: maxContentLength가 1900으로 설정되어 있지만, 마크다운 코드 블록 백틱()과 "(truncated)" 문자열의 길이를 고려하지 않고 있습니다. format() 메서드에서 "" + stack + "```"를 추가하고, truncate()에서 "\n...(truncated)"를 추가하면 실제로는 2000자를 초과할 수 있습니다.
(2) 영향: 디스코드 API의 2000자 제한을 초과하여 메시지 전송이 실패할 수 있습니다.
(3) 수정 제안: 마크다운 구문과 truncated 메시지의 길이를 고려하여 maxContentLength를 조정하거나, truncate() 로직을 개선하여 최종 메시지가 2000자를 넘지 않도록 보장하세요.
| if (s.length() <= max) return s; | |
| return s.substring(0, max) + "\n...(truncated)"; | |
| // 최종 문자열 길이가 max를 넘지 않도록, 잘림 표시 문자열 길이를 포함해 계산 | |
| String suffix = "\n...(truncated)"; | |
| int suffixLen = suffix.length(); | |
| // 이미 max 이하이면 그대로 반환 | |
| if (s.length() <= max) return s; | |
| // max가 suffix 길이보다 작거나 같은 극단적인 설정에 대한 방어 코드 | |
| if (max <= suffixLen) { | |
| // 최소한 0 이상 max 이하 범위에서 substring 수행 | |
| int end = Math.max(0, max); | |
| return s.substring(0, end); | |
| } | |
| int end = max - suffixLen; | |
| if (end < 0) { | |
| end = 0; | |
| } | |
| return s.substring(0, end) + suffix; |
변경된 점